Fix finally-region cloning losing break/continue/parentTryOp wiring#233
Merged
Conversation
This was referenced Jul 14, 2026
ASDAlexander77
added a commit
that referenced
this pull request
Jul 15, 2026
A follow-up static audit of LowerToAffineLoops.cpp, LowerToLLVM.cpp, and the TypeScriptExceptionPass files (beyond the 10 findings already closed in PRs #230/#232/#233/#234/#235) found and fixes 7 more confirmed bugs: - SwitchStateOpLowering: generator yield/resume state labels were collected into a SmallPtrSet<Operation*,16>, whose iteration order is only insertion order below 16 entries; past that it silently falls back to pointer-hash order, scrambling the positional switch dispatch used by generator resume. Replaced with an order-preserving SmallVector. - StringConcatOpLowering: the stack-allocation path sized its Alloca using a pointer type instead of i8, over-allocating by sizeof(ptr) for every multi-argument string concatenation (e.g. console.log with 2+ args). - ArraySpliceOpLowering: mixed genuine MLIR index-dialect ops with already-LLVM-converted operands, a type mismatch that made every Array.prototype.splice() call fail to lower correctly (the only .splice() call in the test suite was commented out because of this). Reworked to stay consistently in the LLVM-converted domain, matching every sibling array-mutation lowering. - GlobalOpLowering: the side-effect enumeration deciding whether a global initializer can be inlined as a constant vs. must run as a real constructor was missing several ops with real side effects (array push/unshift/splice/pop/shift, delete, set-length, string concat/ char-to-string). - LandingPadFixPass: MadeChange was set unconditionally for every landingpad visited, even when nothing was rewritten, causing needless analysis invalidation on every EH-containing function on every compile. - Win32ExceptionPass: the PHI-user removal loop erased only the first PHI user found then unconditionally erased the underlying value, violating LLVM's no-remaining-uses invariant when more than one PHI referenced the same to-be-removed value (own TODO comment flagged this as incomplete). - Win32ExceptionPass: getThrowFn's fallback path created a stray _CxxThrowException Function without attaching it to the module. Added 00array_splice.ts (JIT + AOT) covering shrink/grow/equal-size splice cases, since this path had zero prior test coverage. Full ctest suite green: 692/692. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #232, addressing the last actionable finding (item 5) from the
LowerToAffineLoops.cpp/LowerToLLVM.cpplowering-passes review (docs/LowerToAffineLoops-LowerToLLVM-review.md).TryOpLowering'sfinallyregion cloning —cloneRegionBeforeis called twice to build the normal-exit and return-cleanup copies offinally, producing entirely newOperation*instances each time. AnytsContextside-table entry (jumps[]forbreak/continue,parentTryOp[]for a nestedtry) keyed on the original region's ops — populated by walking that region before cloning — was silently missing for both clones; only the third, inlined copy (which preserves original identity) kept correct wiring. Fixed by passing an explicitmlir::IRMappingto eachcloneRegionBeforecall and re-propagating matching side-table entries from each old op onto its clone via the mapping'sgetOperationMap().trynested directly inside afinallyblock failed to compile at all (error: failed to legalize operation 'ts.Try' that was explicitly marked illegal), independent of break/continue. Root cause: the dialect-conversion driver legalizes a cloned nestedTryOprecursively while the outerTryOp's own pattern application is still on the stack, and refuses to reapply the same sharedPatterninstance recursively without an explicitsetHasBoundedRewriteRecursion()opt-in — which noTsPatternhad. Fixed by givingTryOpLoweringits own constructor that opts in (safe here: each recursive application strictly consumes oneTryOp).break/continueinside atrybody (as opposed to directly insidefinally) does not run the enclosingfinallyblock before jumping — reproduces a JIT crash on unmodifiedmain, predates all three passes of this review. Out of scope for this change; the new test only covers break/continue placed directly insidefinally, which works correctly.Test plan
cmake --build __build/tslang/windows-msbuild-2026-debug --config Debug -j8— clean buildctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8— 690/690 passed (JIT + AOT)00try_finally_break_continue.ts: break directly infinally, continue directly infinally, and a nestedtry/catchinsidefinally— each exercised across multiple loop iterations to catch "only the first/last clone copy is wired correctly" bugs. JIT + AOT green.